bugfix(contain): Restore retail compatibility after crash fix in OpenContain::processDamageToContained by Caball009 · Pull Request #2427 · TheSuperHackers/GeneralsGameCode

Caball009 · 2026-03-08T17:59:12Z

Closes Restore retail compatibility after crash fix in OpenContain::processDamageToContained #2223
Follow up for [GEN][ZH] Fix crash in OpenContain::processDamageToContained() when killed occupants kill the host container #1019

The way the crash fix was implemented in #1019 assumes the container size is either unchanged or 0. This is not the case in the attached replay in #2223.

Gameplay without this PR:

processDamageToContained_before2.mp4

Gameplay with this PR:

processDamageToContained_after2.mp4

Due to a different bug, a GLA Worker enters the unmanned Emperor Overlord but becomes part of the crew instead of the pilot. When the Emperor gets destroyed it first removes its Gattling turret and so the container size has changed. The Worker is still inside, though, so breaking out of the loop is not allowed.

TODO:

Replicate in Generals.
Polish comments.

greptile-apps · 2026-03-08T18:03:55Z

Greptile Summary

This PR fixes a retail-compatibility regression introduced by #1019, where the previous crash fix assumed a container size change during processDamageToContained always meant the list had been fully emptied (and therefore broke out of the loop). The broken assumption caused passengers to be skipped when a container's size shrank for any reason — e.g. the Emperor Overlord losing its Gattling turret occupant before the Worker is processed.

Key changes:

Replaces the break-on-size-change guard with a snapshot-copy approach: the m_containList is copied into a local array (stack-allocated Object*[16] for small containers, std::vector<Object*> for larger ones) before iteration begins, so structural changes to the live list during damage processing cannot corrupt the loop.
Extracts the per-object damage loop into a new #if RETAIL_COMPATIBLE_CRC-guarded helper processDamageToContainedInternal to avoid code duplication between the two size branches.
The non-retail (#else) path is untouched in its core logic; only the deathType local variable is hoisted for DRY-ness.
The existing DEBUG_ASSERTCRASH confirming m_containListSize == m_containList.size() acts as a pre-condition guard before the copy.

Confidence Score: 5/5

Safe to merge — the fix is logically sound and the only finding is a minor access-modifier style issue

All remaining feedback is P2 style (the processDamageToContainedInternal public-vs-private placement). The core logic — snapshot copy, stack/heap split at 16, deferring object removal to game-logic finalization — is correct and matches the direction agreed upon in prior review threads.

Both OpenContain.h headers place processDamageToContainedInternal in the public section; consider moving it to private

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Include/GameLogic/Module/OpenContain.h	Adds processDamageToContainedInternal declaration under #if RETAIL_COMPATIBLE_CRC; placed in public section instead of private
GeneralsMD/Code/GameEngine/Include/GameLogic/Module/OpenContain.h	Mirrors Generals header change — same public placement issue for processDamageToContainedInternal
Generals/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	Replaces the fragile size-change break guard with a snapshot-copy approach; stack array used for containers <16, std::vector for larger; logic is correct
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	Same snapshot approach as Generals; correctly uses m_isBurnedDeathToUnits for death type in Zero Hour; deathType hoisted out of per-object loop for the non-retail path

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[processDamageToContained] --> B{RETAIL_COMPATIBLE_CRC?}
    B -- Yes --> C[Assert m_containListSize == m_containList.size]
    C --> D{m_containListSize < 16?}
    D -- Yes --> E[Stack copy: Object* containCopy-16]
    D -- No --> F[Heap copy: std::vector containCopy]
    E --> G[processDamageToContainedInternal\nstack array, m_containListSize]
    F --> H[processDamageToContainedInternal\nvector data, vector size]
    G --> I[For each object in snapshot:\nattemptDamage + optional kill\nOriginal list NOT modified here]
    H --> I
    B -- No --> J[Swap m_containList into local list\nm_containListSize = 0]
    J --> K[For each object in local list:\nattemptDamage + optional kill\nErase dead objects from local list]
    K --> L[Swap local list back to m_containList\nUpdate m_containListSize]

Prompt To Fix All With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Include/GameLogic/Module/OpenContain.h
Line: 208-210

Comment:
**`processDamageToContainedInternal` should be `private`**

This method is only ever called by `processDamageToContained` within the same class and is an implementation detail not intended for external callers — its "Internal" suffix makes this clear. Declaring it `public` in both `Generals/` and `GeneralsMD/` headers unnecessarily widens the API surface.

```suggestion
private:
#if RETAIL_COMPATIBLE_CRC
	void processDamageToContainedInternal(Object* const* objects, size_t size, Real percentDamage);
#endif
public:
```

The same applies to `GeneralsMD/Code/GameEngine/Include/GameLogic/Module/OpenContain.h` line 222–224.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (7): Last reviewed commit: "Replicated in Generals (manually)." | Re-trigger Greptile}

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp

Caball009 · 2026-03-20T16:04:14Z

I reset the branch and broke the new diff into manageable commits.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp

xezon

If it work, it works.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp

Caball009 · 2026-03-27T16:35:11Z

Addressed feedback.

In the unlikely scenario that the new implementation turns out not to be fully retail compatible, I expect that adding the following code here would fix it:

if (size != m_containListSize)
{
	if (m_containListSize == 0)
		break;

	if (object->isEffectivelyDead())
		continue;

	if (std::find(m_containList.begin(), m_containList.end(), object) == m_containList.end())
		continue;
}

Caball009 · 2026-03-27T21:50:47Z

I can replicate in Generals unless there are other changes desired.

Caball009 added Bug Something is not working right, typically is user facing Major Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour ThisProject The issue was introduced by this project, or this task is specific to this project labels Mar 8, 2026

greptile-apps bot reviewed Mar 8, 2026

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp Outdated Show resolved Hide resolved

Caball009 marked this pull request as draft March 10, 2026 19:29

Caball009 force-pushed the fix_OpenContain_processDamageToContained branch from b533974 to b342a9f Compare March 11, 2026 08:33

Caball009 marked this pull request as ready for review March 11, 2026 08:51

greptile-apps bot reviewed Mar 11, 2026

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp Outdated Show resolved Hide resolved

Mauller reviewed Mar 12, 2026

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp Outdated Show resolved Hide resolved

xezon reviewed Mar 17, 2026

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp Outdated Show resolved Hide resolved

Caball009 added 7 commits March 20, 2026 16:28

Added 'processDamageToContainedInternal' for refactor.

0dcb53d

Moved loop to internal function.

6e5d403

Added call to internal function & cleanup.

38dbc74

Changed comments.

91ac7d0

Added list copy logic.

c64c9a1

Added optimization for small list size.

144e6b4

Hoisted 'isBurnedDeathToUnits' outside of the loop.

4d5d684

Caball009 force-pushed the fix_OpenContain_processDamageToContained branch from 6c35643 to 4d5d684 Compare March 20, 2026 15:46

Caball009 commented Mar 20, 2026

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp Outdated Show resolved Hide resolved

Changed comments.

cebb0a5

xezon approved these changes Mar 23, 2026

View reviewed changes

xezon approved these changes Mar 29, 2026

View reviewed changes

Caball009 added 2 commits March 29, 2026 17:32

Addressed feedback.

a725544

Replicated in Generals (manually).

9ad8a44

Caball009 force-pushed the fix_OpenContain_processDamageToContained branch from 6c196ce to 9ad8a44 Compare March 29, 2026 18:26

xezon merged commit bab63d4 into TheSuperHackers:main Mar 29, 2026
23 checks passed

Caball009 deleted the fix_OpenContain_processDamageToContained branch March 29, 2026 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix(contain): Restore retail compatibility after crash fix in OpenContain::processDamageToContained#2427

bugfix(contain): Restore retail compatibility after crash fix in OpenContain::processDamageToContained#2427
xezon merged 10 commits intoTheSuperHackers:mainfrom
Caball009:fix_OpenContain_processDamageToContained

Caball009 commented Mar 8, 2026 •

edited

Loading

Uh oh!

greptile-apps bot commented Mar 8, 2026 •

edited

Loading

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caball009 commented Mar 20, 2026

Uh oh!

Uh oh!

xezon left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caball009 commented Mar 27, 2026 •

edited

Loading

Uh oh!

Caball009 commented Mar 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Caball009 commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Gameplay without this PR:

Gameplay with this PR:

TODO:

Uh oh!

greptile-apps bot commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caball009 commented Mar 20, 2026

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caball009 commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Caball009 commented Mar 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Caball009 commented Mar 8, 2026 •

edited

Loading

greptile-apps bot commented Mar 8, 2026 •

edited

Loading

Caball009 commented Mar 27, 2026 •

edited

Loading